Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip params when unavailable #39

Merged
merged 2 commits into from
Oct 10, 2019
Merged

Skip params when unavailable #39

merged 2 commits into from
Oct 10, 2019

Conversation

martin308
Copy link
Member

Fixes #31.

Tracing this through, we call this method and action_dispatch.request.parameters is nil. So we fall into this method which eventually ends up calling this lambda which raises an error when raw_post is nil.

I'm not entirely clear what twirp is doing to the request to end up in this state and have struggled to reproduce this in a test. This should prevent the issue though and doesn't seem to cause any other side effects that I could find.

@martin308
Copy link
Member Author

The circle ci build error will be gone once #38 is merged

Copy link
Member

@eanakashima eanakashima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but 😑 for the fact that this happens — looking at request params feels like a pretty standard thing to do in Rails. Maybe worth adding to the comment that twirp is the only reason we think we'd be likely to get into this state, just for future travelers.

@toshok
Copy link

toshok commented Oct 9, 2019

yeah, lgtm, although without more comments inline I'd have to git blame and grovel around in history to find this conversation.

@martin308
Copy link
Member Author

Added additional information to the comment, I'll wait until #38 has been merged though so that I can test that it can push to ruby gems correctly when I release this

@martin308 martin308 merged commit 7fe276c into master Oct 10, 2019
@martin308 martin308 deleted the martin308.skip-params branch October 10, 2019 21:48
ajvondrak added a commit to ajvondrak/beeline-ruby that referenced this pull request Feb 22, 2020
* Remove extraneous spans that were being generated due to the
  ActiveSupport::Notifications subscriptions. There's no need to
  actually configure these from the tests, since we already have the
  active_support_spec.rb, and it's not like the config in the
  rails_spec.rb even matched the one from the Rails generator or
  anything. These just get in the way of testing the Rails integration,
  since the custom fields we'd want to make assertions about only wind
  up on the root span anyway.

* Factor out assertions into a set of shared examples, and actually
  start making assertions about the `request.{action,controller,route}`
  fields that the integration adds (which weren't even being tested for
  previously 😱).

* Rework the "bad request" tests that reproduce honeycombio#49. These tests will
  now break against the old implementation, since they assert that
  routing information is still present (whereas honeycombio#50 wipes the fields out
  with error messages). There are ways to get the routing information
  without ever trying to parse the GET/POST parameters, so we can still
  save this use case. Furthermore, the only Rails 5+ specific changes
  are to whether or not Rails responds with an HTTP 400, so we needn't
  wipe out the entire `describe` block for Rails < 5.

* Add tests to reproduce the issue described in honeycombio#62, where unrecognized
  routes may cause the old implementation to erroneously fall back to
  the GET/POST parameters for routing information.

* Add tests that reproduce honeycombio#31. The bug is actually in the twirp gem,
  whose middleware fails to rewind the `rack.input` after reading it.
  I've verified these regression tests against the old version of the
  Rails integration that didn't check `if request.raw_post`, and they
  fail appropriately. I wanted to have this test in here to have
  confidence in changing the implementation of the Rails integration.
  The `raw_post` workaround of honeycombio#39 is unnecessary if we actually never
  look at the GET/POST parameters in the first place (no parsing, no
  problem).
ajvondrak added a commit to ajvondrak/beeline-ruby that referenced this pull request Feb 22, 2020
This refactoring avoids ever parsing GET/POST parameters to obtain the
Rails routing information required for the `request.action`,
`request.controller`, and `request.route` fields.

This is accomplished in two ways:

1. Use `ActionDispatch::Request#path_parameters` instead of
   `ActionDispatch::Request#params`. The former is the subset of data we
   need for `request.action` + `request.controller`, and in fact are the
   values merged into the latter hash after `#params` parses the normal
   GET/POST parameters. So we can avoid the extraneous parsing by just
   getting the path parameters straight from the horse's mouth.

2. Call `ActionDispatch::Journey::Router#recognize` on a simplified
   request that *only* contains the necessary routing information. The
   `#recognize` method will yield the `ActionDispatch::Request#params`
   when it matches a route, which again triggers the GET/POST parsing.
   But we don't care about the HTTP parameters. So instead I copied the
   approach of `ActionDispatch::Routing::RouteSet#recognize_path`, which
   reconstructs a simplified request with just the information that it
   needs. (Unfortunately we can't use `#recognize_path` directly because
   instead of returning the route it returns the path parameters, which
   we can already get directly from `#path_parameters`.)

This accomplishes several things:

* Fixes honeycombio#62, since we don't get a chance to erroneously fall back to
  GET/POST parameters in the event that the routing information is
  missing (due to requests to unrecognized routes).

* Fixes honeycombio#31, since the bug ultimately comes from the twirp gem causing
  POST parameter parsing to fail. But if we never parse the HTTP
  parameters, no problem. This is a more direct workaround than honeycombio#39, but
  ultimately is a problem that should be fixed upstream.

* Fixes honeycombio#49, since the parameter encoding won't matter if we never try
  to parse them. Renders honeycombio#50 unnecessary, since we won't have a chance
  to trigger the exceptions in question.

* Gives us more data in more situations. The new implementation of the
  `request.route` field now comes up non-nil in the event of errors
  because we use the `ActionDispatch::Request#original_fullpath` and not
  the `#path_info` that gets [rewritten by
  `ActionDispatch::ShowExceptions`](https://github.com/rails/rails/blob/758e4f8406e680a6cbf21b170749202c537a2576/actionpack/lib/action_dispatch/middleware/show_exceptions.rb#L49).

* Gives us more data in more Rails versions. With the addition of actual
  tests for the field values, it became clear that `request.route` was
  missing in Rails 4, simply because of an interface change introduced
  in Rails 5.
martin308 pushed a commit that referenced this pull request Mar 9, 2020
This refactoring avoids ever parsing GET/POST parameters to obtain the
Rails routing information required for the `request.action`,
`request.controller`, and `request.route` fields.

This is accomplished in two ways:

1. Use `ActionDispatch::Request#path_parameters` instead of
   `ActionDispatch::Request#params`. The former is the subset of data we
   need for `request.action` + `request.controller`, and in fact are the
   values merged into the latter hash after `#params` parses the normal
   GET/POST parameters. So we can avoid the extraneous parsing by just
   getting the path parameters straight from the horse's mouth.

2. Call `ActionDispatch::Journey::Router#recognize` on a simplified
   request that *only* contains the necessary routing information. The
   `#recognize` method will yield the `ActionDispatch::Request#params`
   when it matches a route, which again triggers the GET/POST parsing.
   But we don't care about the HTTP parameters. So instead I copied the
   approach of `ActionDispatch::Routing::RouteSet#recognize_path`, which
   reconstructs a simplified request with just the information that it
   needs. (Unfortunately we can't use `#recognize_path` directly because
   instead of returning the route it returns the path parameters, which
   we can already get directly from `#path_parameters`.)

This accomplishes several things:

* Fixes #62, since we don't get a chance to erroneously fall back to
  GET/POST parameters in the event that the routing information is
  missing (due to requests to unrecognized routes).

* Fixes #31, since the bug ultimately comes from the twirp gem causing
  POST parameter parsing to fail. But if we never parse the HTTP
  parameters, no problem. This is a more direct workaround than #39, but
  ultimately is a problem that should be fixed upstream.

* Fixes #49, since the parameter encoding won't matter if we never try
  to parse them. Renders #50 unnecessary, since we won't have a chance
  to trigger the exceptions in question.

* Gives us more data in more situations. The new implementation of the
  `request.route` field now comes up non-nil in the event of errors
  because we use the `ActionDispatch::Request#original_fullpath` and not
  the `#path_info` that gets [rewritten by
  `ActionDispatch::ShowExceptions`](https://github.com/rails/rails/blob/758e4f8406e680a6cbf21b170749202c537a2576/actionpack/lib/action_dispatch/middleware/show_exceptions.rb#L49).

* Gives us more data in more Rails versions. With the addition of actual
  tests for the field values, it became clear that `request.route` was
  missing in Rails 4, simply because of an interface change introduced
  in Rails 5.

---

Revamp the Rails integration's tests



* Remove extraneous spans that were being generated due to the
  ActiveSupport::Notifications subscriptions. There's no need to
  actually configure these from the tests, since we already have the
  active_support_spec.rb, and it's not like the config in the
  rails_spec.rb even matched the one from the Rails generator or
  anything. These just get in the way of testing the Rails integration,
  since the custom fields we'd want to make assertions about only wind
  up on the root span anyway.

* Factor out assertions into a set of shared examples, and actually
  start making assertions about the `request.{action,controller,route}`
  fields that the integration adds (which weren't even being tested for
  previously 😱).

* Rework the "bad request" tests that reproduce #49. These tests will
  now break against the old implementation, since they assert that
  routing information is still present (whereas #50 wipes the fields out
  with error messages). There are ways to get the routing information
  without ever trying to parse the GET/POST parameters, so we can still
  save this use case. Furthermore, the only Rails 5+ specific changes
  are to whether or not Rails responds with an HTTP 400, so we needn't
  wipe out the entire `describe` block for Rails < 5.

* Add tests to reproduce the issue described in #62, where unrecognized
  routes may cause the old implementation to erroneously fall back to
  the GET/POST parameters for routing information.

* Add tests that reproduce #31. The bug is actually in the twirp gem,
  whose middleware fails to rewind the `rack.input` after reading it.
  I've verified these regression tests against the old version of the
  Rails integration that didn't check `if request.raw_post`, and they
  fail appropriately. I wanted to have this test in here to have
  confidence in changing the implementation of the Rails integration.
  The `raw_post` workaround of #39 is unnecessary if we actually never
  look at the GET/POST parameters in the first place (no parsing, no
  problem).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActionDispatch::Http::Parameters::ParseError with Rack app mounted in Rails
3 participants